Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix historical event subscriptions when made as first provider request #3401

Closed

Conversation

cgewecke
Copy link
Collaborator

@cgewecke cgewecke commented Mar 3, 2020

Description

PR targets #3190 (because it fixes a Websocket bug)

Resolves a race condition that arises when the first request made by a Websocket provider is a subscription to an event which includes a request for historical logs.

Web3 was deleting the fromBlock parameter made for the historical logs request while the Websocket connection established itself, resulting in the "missing logs" bug reported in #3389.

Commit 64c1302 is a test that can be seen failing in Travis here
Commit 7f3f42b fixes it by copying the params.

The copied params object looks like....

{
 "fromBlock": "0x0",
 "topics": [
  "0x929af6b98ce8455e51b1e90cceeeec03ce38d7c7396f1f0b0233d043c898a29c",
  null
 ],
 "address": "0xc30a9b6f84a3d660585437a00ddd0090773ea9dc"
}

Fixes #3389

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@cgewecke cgewecke added 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Mar 3, 2020
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 88.12% when pulling 7f3f42b on pri/fix-from-block-race into 3ab0779 on provider-related-improvements.

@nivida nivida added the Bug Addressing a bug label Mar 12, 2020
@nivida
Copy link
Contributor

nivida commented Mar 12, 2020

@cgewecke Is it not possible to have the base branch defined as 1.x? (I think we shouldn't blow up the PRI branch more)

@nivida nivida requested a review from holgerd77 March 12, 2020 20:44
@cgewecke
Copy link
Collaborator Author

@nivida Yes, that's one possibility. The main reason to point it at #3190 is that it will need to be rebased into #3190 anyway since it touches the same files, and there's a growing list of merge conflicts there.

But maybe it makes sense to go through #3190 and pull out everything that could go into 1.x on it's own. There are tests and fixes for misc bugs discovered in the course of the work there which could be addressed as separate items.

That would reduce the size / complexity of #3190 further and perhaps it could also be re-opened as a smaller set of commits that a reviewer can easily follow.

Will wait for feedback from @holgerd77 and @ryanio about how best to proceed...

@cgewecke
Copy link
Collaborator Author

cgewecke commented Apr 3, 2020

Closing in favor #3455 (which targets 1.x directly) for simplicity.

This PR is 180+ commits ahead of 1.x & it's simplest to retarget in a fresh request.

@cgewecke cgewecke closed this Apr 3, 2020
@mpetrunic mpetrunic deleted the pri/fix-from-block-race branch October 31, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Bug Addressing a bug Review Needed Maintainer(s) need to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants